Skip to content

Guard packerProc against a missing :packer key on JRuby - #406

Merged
byroot merged 1 commit into
msgpack:masterfrom
youdie006:symbol-unpacker-only-npe
Sep 11, 2026
Merged

Guard packerProc against a missing :packer key on JRuby#406
byroot merged 1 commit into
msgpack:masterfrom
youdie006:symbol-unpacker-only-npe

Conversation

@youdie006

Copy link
Copy Markdown
Contributor

On JRuby, registering a Symbol type with only an unpacker raises:

factory = MessagePack::Factory.new
factory.register_type(0x00, Symbol, unpacker: :to_sym.to_proc)
# Java::JavaLang::NullPointerException:
#   Cannot invoke "org.jruby.runtime.builtin.IRubyObject.isNil()" because "<local7>" is null

CRuby accepts the same call and round-trips (factory.load(factory.dump(:foo)) is "foo"). Registering any other module the same way works on both.

Cause

RubyHash#fastARef returns Java null for an absent key, where CRuby's rb_hash_aref returns Qnil. Factory.java:115 is the one place in registerTypeInternal that dereferences a fastARef result without a null check:

IRubyObject packerProc = options.fastARef(runtime.newSymbol("packer"));   // null when :packer is absent
...
hasSymbolExtType = !packerProc.isNil();                                   // NPE

The other two results in the same method are both guarded — recursiveExtensionArg != null && ... (Factory.java:107) and oversizedIntegerExtensionArg != null && ... (Factory.java:120).

So is this same value, everywhere else it is read. ExtensionEntry.hasPacker (ExtensionRegistry.java:139-141) guards it with exactly the expression this change uses:

public boolean hasPacker() {
  return packerProc != null && !packerProc.isNil();
}

That matters beyond style: extensionRegistry.put(...) on the line directly above already receives this same null and handles it, which is why hasPacker is written that way. Only the hasSymbolExtType assignment was left unguarded.

The CRuby counterpart reaches the same result through Qnil: fc->has_symbol_ext_type = NIL_P(options) || RTEST(packer_proc); (ext/msgpack/factory_class.c:237) — an absent :packer gives Qnil, RTEST is false, and has_symbol_ext_type ends up false. With this change JRuby agrees.

This is not new in #403; that PR moved !packerProc.isNil() from the if condition into the assignment, and the dereference was present in both forms. It is just the most recent commit on the line.

Why packer-less registration is a shape worth supporting

registered_types has a dedicated "unpacker definition only" branch (lib/msgpack/factory.rb:64-67), and #332 was specifically about making a Symbol type with a nil packer work. This is the same shape with the key omitted rather than set to nil.

Verification

Built and run in Docker. The Java sources were compiled with javac and re-jarred for every row, and I md5'd the jar each time to be sure the rebuild actually reached the runtime.

row result
new spec, current master 69 examples, 1 failure (factory_spec.rb:276)
new spec, with the fix 69 examples, 0 failures
revert the fix 69 examples, 1 failure (factory_spec.rb:276)
packerProc != null only, dropping the isNil() half 69 examples, 1 failure (factory_spec.rb:270)

The last row is the useful one: dropping the isNil() half breaks the existing handles Symbol type with packer: nil`` example from #332, so both halves of the expression are doing work and neither test is decorative.

Full suites, each with the pattern Rakefile:57-61 selects for that platform:

engine result
CRuby 3.2.11 (spec/{,cruby/}*_spec.rb) 460 examples, 0 failures, 1 pending
JRuby 9.4 (spec/{,jruby/}*_spec.rb) 414 examples, 0 failures, 12 pending

The C extension is untouched, so the valgrind job has no new path to cover.


Disclosure: I used Claude (an AI assistant) while preparing this change. Every result above I ran and verified myself.

RubyHash#fastARef returns Java null for an absent key, so registering a
Symbol type with only an :unpacker raised NullPointerException.
ExtensionEntry.hasPacker already guards the same value this way.
@byroot

byroot commented Sep 11, 2026

Copy link
Copy Markdown
Member

Thanks for the fix, but please tune down your agent. A wall of text like this for a one liner fix is a huge pain.

@byroot
byroot merged commit db16d5d into msgpack:master Sep 11, 2026
19 checks passed
@youdie006

Copy link
Copy Markdown
Contributor Author

Fair, and noted — I will keep the body proportional to the diff from here. Thanks for merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants